Reviewed 2 of 8 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, 2 of 6 files at r5, all commit messages.
Reviewable status: 6 of 10 files reviewed, 2 unresolved discussions (waiting on @fcanderson)

OpenSim/Simulation/StatesTrajectoryReporter.cpp line 39 at r5 (raw file):

const std::vector<SimTK::State>&
StatesTrajectoryReporter::getStateArray() const {
    return m_states.getStateArray();

Wonder if the name of the method should change?

OpenSim/Simulation/Test/testStatesTrajectory.cpp line 580 at r5 (raw file):

    //    states[5];
    //#endif

Any reason to keep these comments?

Reviewable status: 5 of 10 files reviewed, 2 unresolved discussions (waiting on @aymanhab and @nickbianco)

OpenSim/Simulation/StatesTrajectoryReporter.cpp line 39 at r5 (raw file):

Ah, I think I see what you are suggesting. The return is now a vector<> instead of a SimTK::Array_<>.

I didn't want to use getStateTrajectory() since there is a StateTrajectory class.

The only reason I might not want to use getStateVector() is that name will likely conjure the notion of an object that contains the states at a single time stamp. A SimTK::State object itself is what some might think of as a state vector.

The name that I think is probably most accurate and least likely to be confused with some other entity in this arena is

getVectorOfStateObjects()

since an std::vector<> is returned.

If you agree, I'll go ahead and change the method name to getVectorOfStateObjects().

OpenSim/Simulation/Test/testStatesTrajectory.cpp line 580 at r5 (raw file):

I do not know why I commented out these lines originally. So, I'm glad you spotted them. It likely had to do with the fact that testStatesTrajectory fails for a Debug build. I might have thought that commenting out those lines would allow the test to run successfully. Not sure, but if I did, that was wrong.

I just tested the code with those lines commented out and with those lines restored. Either way, the Debug build fails because of an array index out of bounds exception that is thrown while executing

SimTK_SUBTEST1(testFromStatesStorageInconsistentModel, pre40StoFname);  (testStatesTrajectory.cpp: line 799)

In a release build (either Release or RelWithDebInfo) the test passes because bounds checking is disabled.

Since I didn't write this test, the better thing to do restore the lines that I commented out. I'm not exactly sure what functionality they are testing.

As a general comment, there are a number of OpenSim tests that fail for a Debug build. I haven't reported them because I suspected that you all (the devs) were aware of the failures and had decided to pass on them as non-critical. After talking with @nickbianco , however, it seems that maybe running the tests for a Debug build isn't really part of the development cycle. So maybe you all are not aware of the Debug test failures?

I intended to follow up with @nickbianco on this, but I just haven't had the resources to do it.

A couple minor comments but otherwise looks great. My main question at the end of this was how would API users distinguish if they're using a Full state with discrete variable and modeling options vs. the traditional state that had only continuous variables. Maybe some documentation/disclaimers would do.

Reviewed 4 of 6 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @fcanderson)

OpenSim/Simulation/StatesDocument.h line 221 at r6 (raw file):

significant figures would go unnoticed).

Amazingly clear description @fcanderson Thank you 🥇

OpenSim/Simulation/StatesDocument.cpp line 435 at r6 (raw file):

    // Many other aspects are checked for consistency than just the model
    // name. Those are more easily checked as the doc is parced.

parsed?

OpenSim/Simulation/StatesDocument.cpp line 450 at r6 (raw file):

        SimTK_ASSERT_ALWAYS(false, msg.c_str());
    }

This maybe overstated on my side but we never gave the model name much significance, users easily modify model names in GUI or XML without thinking about downstream effects. Considering that the meat of consistency test is done elsewhere I wonder if you think this check is relevant here.

OpenSim/Simulation/StatesDocument.cpp line 673 at r6 (raw file):

                model, path, traj);
        }
        else {

These if/else(s) typically worry me because I don't know if we covered all cases. but with templates I guess that's the price to pay. Is there a master list of these types that we can refer to in case e.g. Arrays of Vectors, Matrices or SpatialVectors beome valid types?

OpenSim/Simulation/StatesTrajectoryReporter.cpp line 39 at r5 (raw file):

Agreed

Very close, I believe I responded to your questions, let's address the remaining items to get it over the hump. Thank you

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @fcanderson)

OpenSim/Simulation/StatesDocument.cpp line 450 at r6 (raw file):

I'll leave it as is and see if it becomes an issue in actual use cases, at this level of API use, more stringent is better.

OpenSim/Simulation/StatesDocument.cpp line 673 at r6 (raw file):

Thanks @fcanderson I'd add comment to the block in the cpp block so that future maintainers understand where this list came from and why.

Thanks @fcanderson and I agree completely with your assessment regarding the holes in the current system before this PR, and appreciate the work you've done to enable this important functionality.

My question was regarding the StateTrjectory class/methods e.g. exportToTable has no way to tell where it came from, could it export discrete variables or modeling-options? Maybe it doesn't matter (that's a totally satisfactory resolution) or if not we could maintain a flag for book-keeping. Mainly asking for clarification for users who are already utilizing the StatesTrajectory vehicle elsewhere. If that's a more involved discussion or something that you resolved earlier or better done on Zoom I'm totally open. Thanks again.

Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @fcanderson)

My understanding is that StatesTrajectory generally gets the states from a StatesTrajectoryReporter, in which case the trajectory of SimTK::State objects it contains will have valid values for all state categories (continuous, discrete, modeling). If, however, StatesTrajectory gets its states from a saved file, then any instance of a SimTK::State that is based only on reading that file will only have valid values for the Continuous states; the discrete variables and modeling options will be left uninitialized. Valid values of discrete and modeling states, in this case, will have to come from another source (e.g., another input file).

A flag in the StatesTrajectory to indicate whether it contains an entirely valid trajectory of SimTK::State objects seems like it could be a useful thing. If the origin of the trajectory was from a StatesTrajectoryReport, the flag is set to COMPLETE. On the other hand, if the origin of the trajectory was from file, then the flag is set to INCOMPLETE.

In its current form, StatesTrajectory doesn't export discrete variables or modeling options. It could be modified to do so, however. OpenSim::Component now has the infrastructure to efficiently get or set each state variable (continuous, discrete, or modeling) individually from or into a SimTK::Array_SimTK::State by specifying the variable's path. OpenSim::Component keeps an std::map for each state category using path as the hash index.

Enhancing StatesTrajectory would be a natural step to take except for one road block (that I am aware of). TimeSeriesTable does not currently have a way to co-mingle variables of different types. I really don't like the idea of serializing to multiple files, one for each type encountered (one for the int's, one for double's, one for Vec3's, etc.). That seems like a recipe for a logistical nightmare. How do you manage to keep track of all the files that go together?

The appeal of using XML, at least as for an initial pass at this functionality, is that the xml attribute mechanism was a really natural way to encode variable type. AND, SimTK::Xml already knows how to serialize and deserialize ALL possible variable types that could be encountered in Simbody (matrices, SpatialVec, etc). Using XML presented a clear path to getting all states in ONE file with a model name, appropriate precision, time stamp, and note.

If .ostate file size becomes an issue, I do think pursuing binary serialization would be a productive way to go. To get this functionality, augmenting the appropriate Simbody classes I think should be the point of attack. There would just need to be a flag that is set in SimTK::Xml to signal binary serialization, and each concrete SimTK::Value type would need to know how to stream itself in binary form. The functionality may actually already exist. I haven't looked into it.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @fcanderson)

A follow-up:

1. I read about binary XML formats. It appears like a much debated topic. Most people, it seems, are not in favor of binary because it removes one of the hallmark design characteristics of XML -- it is user readable and editable with common editors.
2. A good alternative to using some kind of binary XML to reduce file size is using compression (e.g., zip'ing).
3. In StatesTrajectory.h, I do make a disclaimer about using StatesTrajectory::createFromStorage() and createfromStatesTable() as a deserialization approach.
   See https://github.com/fcanderson/opensim-core/blob/ostates_2/OpenSim/Simulation/StatesTrajectory.h#L314

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @fcanderson)

Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on @aymanhab)

OpenSim/Simulation/StatesDocument.cpp line 673 at r6 (raw file):

Done.

@aymanhab I believe I've addressed all the requested changes.

You'll notice that I pushed 1 or 2 more commits. I took the liberty of adjusting the documentation to reflect that both std::vecter<SimTK::State> or SimTK::Array_<SimTK::State> will work as in interface to class StatesDocument.

If all looks good to you, then it looks good to me.

Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on @aymanhab)

Thanks @fcanderson Looks great. Please update the changelog and we're done. Great work 👍

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @fcanderson)

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @fcanderson)